-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce animal sniffer #2006
Introduce animal sniffer #2006
Conversation
…ude classes which must not be run on Android.
…id breaking Android builds which cannot process these methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about animalsniffer
. Really nice to prevent these issues!
@@ -98,6 +99,14 @@ dependencies { | |||
testRuntime configurations.compileOnly | |||
|
|||
testUtil sourceSets.test.output | |||
|
|||
signature 'org.codehaus.mojo.signature:java18:1.0@signature' | |||
signature 'net.sf.androidscents.signature:android-api-level-24:7.0_r2@signature' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for my understanding, this says that we are supporting Android API levels 24 and over? If so, we should document that somewhere as well. In the Twitter thread, they proposed level 21 and over. If we set this to 21, do we need to make a lot of changes? Iirc, Mockito mostly builds in Google so I think we would be able to do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should, indeed. Android level 21 does not support types such as Optional
or Function
. We can go that far but it's already part of the public API. I guess 3.5.0 is not super-wide spread yet but I'd like to avoid releasing 4.0 just for that.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, iirc Mockito 3.4.0 builds just fine internally and also includes Optional. I can take a look tomorrow as to why that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks though, that Android can easily desugar these few types: https://developer.android.com/studio/write/java8-support - it only does not work with method handle types which are not exposed. I just asked for a validation of this branch of the user who reported this. By requiring API level 24, we do not leak API that cannot be desugared at least. But yes, we should document this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, with code shrinking, it should already work with 3.5.0: https://developer.android.com/studio/build/shrink-code#shrink-code
Codecov Report
@@ Coverage Diff @@
## release/3.x #2006 +/- ##
=================================================
+ Coverage 84.86% 84.90% +0.04%
Complexity 2692 2692
=================================================
Files 324 324
Lines 8146 8161 +15
Branches 972 972
=================================================
+ Hits 6913 6929 +16
+ Misses 965 964 -1
Partials 268 268
Continue to review full report at Codecov.
|
Doesn't Android consume JCenter? |
Introduces animal sniffer with exclusion of inline-mock-maker classes which would never be present on Android. Avoids calling invoke/invokeExact methods of handles directly but rather puts the invocations into generated code to avoid breaking Android builds.